Skip to content

Conversation

@justinpakzad
Copy link

Updated _DAG_HASH_ATTRS to use timetable instead of schedule . As noted in the TODO, for the Task-SDK we should be hashing on timetable now, not schedule.

I verified the existing unit tests passed. I wasn't sure if this needed any tests since it's swapping functionally equivalent attributes. Happy to add some if needed.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg
Copy link

boring-cyborg bot commented Dec 26, 2025

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@justinpakzad justinpakzad force-pushed the fix/hash-timetable-instead-of-schedule branch from 86c58f0 to a2a4afb Compare December 26, 2025 03:45
@uranusjr
Copy link
Member

This is not enough because timetable classes don’t generally implement __hash__.

@justinpakzad
Copy link
Author

justinpakzad commented Dec 26, 2025

This is not enough because timetable classes don’t generally implement __hash__.

Hey, thanks for pointing that out.

I was looking into it a bit more and found that timetables using @attrs.define (like CronDataIntervalTimetable and DeltaDataIntervalTimetable) still aren't directly hashable, but the DAG's hash method falls back to repr(), which works because @attrs.define generates a proper repr with the actual values (e.g., CronDataIntervalTimetable(expression='@daily', timezone=Timezone('UTC'))).

Where it seems to fail is with simple timetables like NullTimetable, OnceTimetable, and ContinuousTimetable that don't use @attrs.define, so their repr() includes memory addresses, which causes identical DAGs to have different hashes and will also fail the eq check.

I think adding @attrs.define or just defining __repr__ for those classes would sort this issue out.

@justinpakzad justinpakzad force-pushed the fix/hash-timetable-instead-of-schedule branch from a2a4afb to 0ab5d60 Compare December 30, 2025 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants